Skip to content

Conversation

@LeonardHerbst
Copy link
Contributor

Contribution description

This introduces a new driver, a test for the driver, and a new SAUL category ID.

Hall effect sensor / magnetic rotary encoder are often attached directly to the shaft of a motor but can also be bough and used independently.
This driver is not specific to one model sensor. The driver can be used to measure RPM and revolutions since the last readout.

The test just periodically prints the RPM and revolutions since the last read.

The only other drivers measuring angular velocity were gyro drivers which have a SAUL category ID of there own. Therefore I introduced SAUL_SENSE_SPEED.

Testing procedure

I connected a sensor and ran the provided test and tested it form the shell via the SAUL adaptation.

@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer Area: sys Area: System Area: Kconfig Area: Kconfig integration labels Oct 17, 2025
@crasbe crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 17, 2025
@riot-ci
Copy link

riot-ci commented Oct 17, 2025

Murdock results

✔️ PASSED

a1bd6f7 Added a inc_encoder backend to the saul driver test

Success Failures Total Runtime
10564 0 10564 08m:54s

Artifacts

Copy link
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Enoch247
Copy link
Contributor

I only briefly (like 5 minutes) looked at this, but it reminds me of the periph_qdec module. Would it make since to harmonize the two modules in the same way that soft_uart/periph_uart and soft_spi/periph_spi are? Or maybe the periph_qdec module can be brought into this API as a different backend?

@LeonardHerbst
Copy link
Contributor Author

I only briefly (like 5 minutes) looked at this, but it reminds me of the periph_qdec module. Would it make since to harmonize the two modules in the same way that soft_uart/periph_uart and soft_spi/periph_spi are? Or maybe the periph_qdec module can be brought into this API as a different backend?

Thanks I overlooked periph_qdec! I think bringing it in as an optional backend is the way to go.

@LeonardHerbst LeonardHerbst requested a review from crasbe November 10, 2025 08:17
Comment on lines +63 to +64
ztimer_periodic_init(ZTIMER_USEC, &dev->rpm_timer, _rpm_calc_timer_cb, (void *) dev,
CONFIG_INC_ENCODER_HARDWARE_PERIOD_MS * US_PER_MS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it actually necessary to use the microsecond timer here? It prevents deeper sleep modes on most microcontrollers 🤔

Comment on lines +91 to +93
#else
# define INC_ENCODER_PARAMS {}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably unncessary, as this wouldn't work without a software or hardware interface anyways.

* the detected rotation direction will be reversed.
*
* The driver provides functions to read the current RPM
* and the total number of revolutions (in hundredths) since the last measurement.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like "in one hundreths of a revolution" as in "1/100th revolution" 🤔

* This driver implements the SAUL sensor interface.
* It provides two SAUL devices:
* - One for reading the current RPM
* - One for reading the total number of revolutions in hundredths since the last read.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

SAUL_SENSE_ID_PH, /**< sensor: pH */
SAUL_SENSE_ID_POWER, /**< sensor: power */
SAUL_SENSE_ID_SIZE, /**< sensor: size */
SAUL_SENSE_ID_SPEED, /**< sensor: speed */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SAUL_SENSE_ID_SPEED, /**< sensor: speed */
SAUL_SENSE_ID_SPEED, /**< sensor: speed */

/** sensor: size */
SAUL_SENSE_SIZE = SAUL_CAT_SENSE | SAUL_SENSE_ID_SIZE,
/** sensor: speed */
SAUL_SENSE_SPEED = SAUL_CAT_SENSE | SAUL_SENSE_ID_SPEED,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SAUL_SENSE_SPEED = SAUL_CAT_SENSE | SAUL_SENSE_ID_SPEED,
SAUL_SENSE_SPEED = SAUL_CAT_SENSE | SAUL_SENSE_ID_SPEED,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: SAUL Area: Sensor/Actuator Uber Layer Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants